-
Notifications
You must be signed in to change notification settings - Fork 2k
[Improve][Transform-V2] Improve sql transform exception to locate error expression #9227
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
This pull request addresses issue #9125 by enhancing error handling in the SQL transformation logic. Key changes include:
- Wrapping the SQL expression evaluation in a try-catch block to throw a more specific exception.
- Introducing new exception classes and error codes to better pinpoint errors in SQL processing.
- Adding new files for SqlTransformException and SqlTransformErrorCode to support the improved exception handling.
Reviewed Changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated no comments.
File | Description |
---|---|
seatunnel-transforms-v2/src/main/java/org/apache/seatunnel/transform/sql/zeta/ZetaSQLEngine.java | Added exception handling for SQL expression evaluation and improved error reporting. |
seatunnel-transforms-v2/src/main/java/org/apache/seatunnel/transform/exception/SqlTransformException.java | New exception class for SQL transformation errors. |
seatunnel-transforms-v2/src/main/java/org/apache/seatunnel/transform/exception/SqlTransformErrorCode.java | New error code enum defining the SQL transformation error codes. |
Comments suppressed due to low confidence (1)
seatunnel-transforms-v2/src/main/java/org/apache/seatunnel/transform/sql/zeta/ZetaSQLEngine.java:295
- Consider adding a space after the comma in the error message for improved readability, e.g., "Failed to execute sql expression, the error expression is %s".
String.format("Failed to execute sql expression,the error expression is %s", expression),
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please update unit test.
|
||
import org.apache.seatunnel.common.exception.SeaTunnelErrorCode; | ||
|
||
public enum SqlTransformErrorCode implements SeaTunnelErrorCode { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please reuse
Line 33 in 3783816
public class TransformCommonError { |
…ansforCommonError
if (e instanceof TransformException) { | ||
throw e; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why we should special care TransformException
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I previously thought that the computeForValue() method threw a TransformException(), so I should throw it outward.But I found this exception can be merged in the upper layer.
@@ -67,4 +68,11 @@ public static TransformException cannotFindInputTableError(String transform, Str | |||
params.put("transform", transform); | |||
return new TransformException(INPUT_TABLE_NOT_FOUND, params); | |||
} | |||
|
|||
public static TransformException sqlTransformError(String transform, String expression) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
public static TransformException sqlTransformError(String transform, String expression) { | |
public static TransformException sqlExpressionError(String transform, String expression) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
get it
"The input table '<table>' of '<transform>' transform not found in upstream schema"), | ||
EXPRESSION_EXECUTE_ERROR( | ||
"TRANSFORM_COMMON-06", | ||
"The expression '<expression>' of '<transform>' transform execute failed"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
"The expression '<expression>' of '<transform>' transform execute failed"); | |
"The expression '<expression>' of SQL transform execute failed"); |
The transform should always be SQL.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
get it
+ "CAST(`FIELD1` AS STRING) AS FIELD1, " | ||
+ "CAST(`FIELD1` AS decimal(22,4)) AS FIELD2, " | ||
+ "CAST(`FIELD3` AS decimal(22,0)) AS FIELD3 " | ||
+ "from dual"; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we should handle where
statement too.
2.remove throw TransformException 3.rename sqlTransformError to sqlExpressionError 4.modify EXPRESSION_EXECUTE_ERROR description 5.add where statement processing
return null; | ||
} | ||
} catch (Exception e) { | ||
throw TransformCommonError.sqlWhereStatementError(selectBody.getWhere().toString()); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We need add the exception into cause stack. Please refer
seatunnel/seatunnel-common/src/main/java/org/apache/seatunnel/common/exception/CommonError.java
Line 66 in 454a88f
String identifier, String operation, String fileName, Throwable cause) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
get it
@@ -411,7 +411,7 @@ public void tesCastBooleanClausesWithField() { | |||
new SeaTunnelRow(new Object[] {Integer.valueOf(1), 0, "false333"})); | |||
} catch (Exception e) { | |||
Assertions.assertEquals( | |||
"ErrorCode:[COMMON-05], ErrorDescription:[Unsupported operation] - Unsupported CAST AS Boolean: false333", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The new exception looks like lose expression value in message. We should fix it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
already complete
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
This PR enhances SQL transform error reporting by adding specific exception wrappers for expression and WHERE failures, introduces new error codes, and updates tests to validate these detailed messages.
- Wraps filter and projection logic in ZetaSQLEngine with try/catch to provide
TRANSFORM_COMMON-06
andTRANSFORM_COMMON-07
errors. - Adds new error codes and constructors in
TransformCommonErrorCode
andTransformException
, plus helper methods inTransformCommonError
. - Updates existing tests and adds a new test in
SQLTransformTest
to assert on the improved error messages.
Reviewed Changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 3 comments.
Show a summary per file
File | Description |
---|---|
seatunnel-transforms-v2/src/main/java/org/apache/seatunnel/transform/sql/zeta/ZetaSQLEngine.java | Wraps filter/project calls to rethrow with context-specific errors |
seatunnel-transforms-v2/src/main/java/org/apache/seatunnel/transform/exception/TransformException.java | Adds constructor to preserve original cause |
seatunnel-transforms-v2/src/main/java/org/apache/seatunnel/transform/exception/TransformCommonErrorCode.java | Defines new error codes for expression and WHERE errors |
seatunnel-transforms-v2/src/main/java/org/apache/seatunnel/transform/exception/TransformCommonError.java | Implements builder methods for the new error codes |
seatunnel-transforms-v2/src/test/java/org/apache/seatunnel/transform/sql/SQLTransformTest.java | Updates expected error messages and adds new error-case tests |
Comments suppressed due to low confidence (1)
seatunnel-transforms-v2/src/test/java/org/apache/seatunnel/transform/sql/SQLTransformTest.java:473
- [nitpick] Consider making this assertion less brittle by matching on the error code and checking that the expression substring appears in the message, rather than comparing the entire error message exactly. This will reduce test failures due to minor formatting changes.
Assertions.assertEquals(...)
@@ -397,7 +397,7 @@ public void tesCastBooleanClausesWithField() { | |||
new SeaTunnelRow(new Object[] {Integer.valueOf(1), 3, "false"})); | |||
} catch (Exception e) { | |||
Assertions.assertEquals( | |||
"ErrorCode:[COMMON-05], ErrorDescription:[Unsupported operation] - Unsupported CAST AS Boolean: 3", | |||
"ErrorCode:[TRANSFORM_COMMON-06], ErrorDescription:[The expression 'cast(`int` AS boolean)' of SQL transform execute failed]", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The expected error message uses lowercase cast
, but the actual exception uses CAST
(uppercase) from the expression's toString()
. Update this assertion to match CAST
for consistency.
"ErrorCode:[TRANSFORM_COMMON-06], ErrorDescription:[The expression 'cast(`int` AS boolean)' of SQL transform execute failed]", | |
"ErrorCode:[TRANSFORM_COMMON-06], ErrorDescription:[The expression 'CAST(`int` AS boolean)' of SQL transform execute failed]", |
Copilot uses AI. Check for mistakes.
@@ -411,7 +411,7 @@ public void tesCastBooleanClausesWithField() { | |||
new SeaTunnelRow(new Object[] {Integer.valueOf(1), 0, "false333"})); | |||
} catch (Exception e) { | |||
Assertions.assertEquals( | |||
"ErrorCode:[COMMON-05], ErrorDescription:[Unsupported operation] - Unsupported CAST AS Boolean: false333", | |||
"ErrorCode:[TRANSFORM_COMMON-06], ErrorDescription:[The expression 'cast(`string` AS boolean)' of SQL transform execute failed]", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The expected error message uses lowercase cast
, but the actual exception uses CAST
. Update this assertion to expect CAST(
string AS boolean)
.
"ErrorCode:[TRANSFORM_COMMON-06], ErrorDescription:[The expression 'cast(`string` AS boolean)' of SQL transform execute failed]", | |
"ErrorCode:[TRANSFORM_COMMON-06], ErrorDescription:[The expression 'CAST(`string` AS boolean)' of SQL transform execute failed]", |
Copilot uses AI. Check for mistakes.
new SeaTunnelRow(new Object[] {1, 123.123, "true"})); | ||
} catch (Exception e) { | ||
Assertions.assertEquals( | ||
"ErrorCode:[TRANSFORM_COMMON-06], ErrorDescription:[The expression 'CAST(`FIELD3` AS decimal (22, 0))' of SQL transform execute failed]", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The spacing in decimal (22, 0)
may not match the expression's toString()
output (likely decimal(22,0)
). Adjust the expected string to CAST(
FIELD3 AS decimal(22,0))
to align with the actual error message.
"ErrorCode:[TRANSFORM_COMMON-06], ErrorDescription:[The expression 'CAST(`FIELD3` AS decimal (22, 0))' of SQL transform execute failed]", | |
"ErrorCode:[TRANSFORM_COMMON-06], ErrorDescription:[The expression 'CAST(`FIELD3` AS decimal(22,0))' of SQL transform execute failed]", |
Copilot uses AI. Check for mistakes.
…ansforCommonError
2.remove throw TransformException 3.rename sqlTransformError to sqlExpressionError 4.modify EXPRESSION_EXECUTE_ERROR description 5.add where statement processing
Purpose of this pull request
resolve issue #9125
Does this PR introduce any user-facing change?
How was this patch tested?
Check list
New License Guide
release-note
.